Hi @duboisph. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/assign @unguiculus

/auto-cc

Also pinging @mneedham since you're this chart's maintainer :)

Rebased & updated to be compatible with latest changes from #20942.

/ok-to-test

@zanhsieh what's next to get this merged?

@duboisph
Get owner to merge this.

@lachie83
@prydonius
@sameersbn
@viglesiasce
@unguiculus
@scottrigby
@mattfarina
@davidkarlsen
@paulczar
@cpanato
@jlegrone
@maorfr
Could you guys LGTM this please? Thanks.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: duboisph
To complete the pull request process, please assign unguiculus
You can assign the PR to them by writing /assign @unguiculus in a comment when ready.

The full list of commands accepted by this bot can be found here.

Since the merge of #21896, I had to rebase again...

@mattfarina, @mneedham, @moxious can you LGTM this please?

@duboisph sorry it took me some time, there's been some helm chart reorganization going on over here. I have feedback for you.

The PR is good, and the feature is valuable. However, due to this comment recently from @mattfarina, we're re-evaluating what we're going to do with this helm chart. This overall repo (not the neo4j chart) is going to be deprecated, and we're looking at moving the Neo4j chart to a new location here: https://github.com/neo4j-contrib/neo4j-helm

What I'm planning right now is to do new development there, and deprecate this chart with a new PR shortly which will redirect users of this chart to go there instead.

Now to the technical specifics of your PR -- adding prometheus configurability is a great idea, the value is clear. The ServiceMonitor part is problematic, because it uses an API which isn't generally available on your average kubernetes cluster by default. In my local testing, with for example vanilla GKE, using the service monitor does the right thing from the YAML perspective, but since the API/entity isn't available, it bounces off without error. I'd prefer not to include resources in this chart that aren't reasonably universal, as inclusion of these types of resources will pidgeonhole the chart into a narrower set of configurations.

Given that -- I've already taken the essence of your idea (basically all of it except the servicemonitor) and merged it into the neo4j-helm repo above, along with a number of other features and goodies that aren't present in this repo.

With all of that in mind -- I want to thank you for taking the time to put this together, it's clearly useful, but with the context of this repo getting deprecated and so on, I'd recommend closing this PR and not merging it.

Hi @moxious, thanks for the follow-up. It's good to see the neo4j chart migrated to it's own repo and looks like there's already quite some improvements too.

Configuring Neo4j and exposing the metrics port(s) was the main reason for the PR, a ServiceMonitor can always be added by other means.

However I'd still love to see the inclusion of an (optional) ServiceMonitor to the chart at some point. It's true, it doesn't use vanilla API's, however the inclusion of the prometheus-operator and it's CRDs isn't that uncommon afaik and a lot of other charts have started to include ServiceMonitor and/or PrometheusRule resources recently. A quick search through this repo will show numerous results, including most common database systems :).

In any case, I appreciate you getting back to me and I'm excited to start migrating our installations to Neo4j 4.0 and the new chart.

This PR can be closed.